[6.17.z] Use Convert2RHEL repofiles instead of deprecated repos for tests - #20889
Conversation
) Signed-off-by: Gaurav Talreja <gtalreja@redhat.com> (cherry picked from commit 0644d11)
|
Reviewer's GuideUpdate Convert2RHEL API tests to obtain repository base URLs by downloading and parsing remote .repo files, replacing use of deprecated hard-coded repository URLs, and add a shared helper for extracting baseurls from repofiles. Sequence diagram for get_baseurl_by_repofile resolving baseurlsequenceDiagram
participant TestCase
participant ContentInfo
participant Requests
participant RepoServer
TestCase->>ContentInfo: get_baseurl_by_repofile(repo_url, verify_ssl)
ContentInfo->>Requests: get(repo_url, verify_ssl, timeout=10)
Requests->>RepoServer: HTTP GET repo_url
RepoServer-->>Requests: 200 OK, .repo content
Requests-->>ContentInfo: response(text)
ContentInfo->>ContentInfo: parse lines, find baseurl=
ContentInfo-->>TestCase: baseurl string
Class diagram for updated content_info utilitiesclassDiagram
class ContentInfo {
+get_repo_files_by_url(url, extension)
+get_repo_files_urls_by_url(url, extension)
+get_repomd(repo_url)
+get_baseurl_by_repofile(repo_url, verify_ssl)
}
class Requests {
+get(url, verify, timeout)
}
ContentInfo ..> Requests : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new
get_baseurl_by_repofileparser is quite strict (line.startswith('baseurl=')), so it will miss common variations likebaseurl = ...or uppercase keys; consider usingconfigparseror a more flexible parse (e.g., splitting on=after checkingline.lower().startswith('baseurl')). - Instead of hard-coding the
timeout=10inrequests.get, consider reusing any existing HTTP helper or making the timeout configurable/centralized to keep network behavior consistent across the module.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `get_baseurl_by_repofile` parser is quite strict (`line.startswith('baseurl=')`), so it will miss common variations like `baseurl = ...` or uppercase keys; consider using `configparser` or a more flexible parse (e.g., splitting on `=` after checking `line.lower().startswith('baseurl')`).
- Instead of hard-coding the `timeout=10` in `requests.get`, consider reusing any existing HTTP helper or making the timeout configurable/centralized to keep network behavior consistent across the module.
## Individual Comments
### Comment 1
<location path="robottelo/content_info.py" line_range="81-90" />
<code_context>
+ for line in response.text.splitlines():
+ line = line.strip()
+
+ if line.startswith('baseurl='):
+ return line.split('=', 1)[1].strip()
+
+ raise ValueError(f'No baseurl found in {repo_url}')
</code_context>
<issue_to_address>
**suggestion:** Handle inline comments and empty values when extracting the baseurl
This will return anything after `baseurl=`, including inline comments (e.g. `baseurl=http://foo # comment`) or an empty value. Consider stripping inline comments first (e.g. split on `#`), ensuring the value is non-empty and URL-like, and otherwise continuing the scan or raising a clearer error to avoid silently returning malformed values.
```suggestion
response = requests.get(repo_url, verify=verify_ssl, timeout=10)
response.raise_for_status()
for line in response.text.splitlines():
line = line.strip()
if line.startswith('baseurl='):
# Extract raw value after "baseurl="
raw_value = line.split('=', 1)[1].strip()
if not raw_value:
# Empty baseurl, continue scanning other lines
continue
# Strip inline comments: everything after '#' is considered a comment
value = raw_value.split('#', 1)[0].strip()
if not value:
# Only comment or whitespace after baseurl=, keep scanning
continue
# Basic sanity check to avoid obviously malformed values
# (most yum/dnf baseurls are URLs or file paths)
if "://" not in value and not value.startswith("file:"):
# Not URL-like, keep scanning for a better candidate
continue
return value
raise ValueError(f'No valid baseurl found in repository metadata from {repo_url}')
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| response = requests.get(repo_url, verify=verify_ssl, timeout=10) | ||
| response.raise_for_status() | ||
|
|
||
| for line in response.text.splitlines(): | ||
| line = line.strip() | ||
|
|
||
| if line.startswith('baseurl='): | ||
| return line.split('=', 1)[1].strip() | ||
|
|
||
| raise ValueError(f'No baseurl found in {repo_url}') |
There was a problem hiding this comment.
suggestion: Handle inline comments and empty values when extracting the baseurl
This will return anything after baseurl=, including inline comments (e.g. baseurl=http://foo # comment) or an empty value. Consider stripping inline comments first (e.g. split on #), ensuring the value is non-empty and URL-like, and otherwise continuing the scan or raising a clearer error to avoid silently returning malformed values.
| response = requests.get(repo_url, verify=verify_ssl, timeout=10) | |
| response.raise_for_status() | |
| for line in response.text.splitlines(): | |
| line = line.strip() | |
| if line.startswith('baseurl='): | |
| return line.split('=', 1)[1].strip() | |
| raise ValueError(f'No baseurl found in {repo_url}') | |
| response = requests.get(repo_url, verify=verify_ssl, timeout=10) | |
| response.raise_for_status() | |
| for line in response.text.splitlines(): | |
| line = line.strip() | |
| if line.startswith('baseurl='): | |
| # Extract raw value after "baseurl=" | |
| raw_value = line.split('=', 1)[1].strip() | |
| if not raw_value: | |
| # Empty baseurl, continue scanning other lines | |
| continue | |
| # Strip inline comments: everything after '#' is considered a comment | |
| value = raw_value.split('#', 1)[0].strip() | |
| if not value: | |
| # Only comment or whitespace after baseurl=, keep scanning | |
| continue | |
| # Basic sanity check to avoid obviously malformed values | |
| # (most yum/dnf baseurls are URLs or file paths) | |
| if "://" not in value and not value.startswith("file:"): | |
| # Not URL-like, keep scanning for a better candidate | |
| continue | |
| return value | |
| raise ValueError(f'No valid baseurl found in repository metadata from {repo_url}') |
|
PRT Result |
Cherrypick of PR: #20869
Problem Statement
Solution
Related Issues
Summary by Sourcery
Update Convert2RHEL API tests to use base URLs derived from remote repofiles instead of hard-coded repository URLs.
Bug Fixes:
Enhancements: